license-fact-providers: Enhance the API to support id-specific license text overrides#11975
license-fact-providers: Enhance the API to support id-specific license text overrides#11975fviernau wants to merge 3 commits into
Conversation
fviernau
force-pushed
the
license-fact-provider-composite
branch
from
79995a8 to
e57d001
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11975 +/- ##
=========================================
Coverage 58.46% 58.46%
Complexity 1813 1813
=========================================
Files 361 361
Lines 13510 13510
Branches 1385 1385
=========================================
Hits 7899 7899
Misses 5115 5115
Partials 496 496
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sschuberth
left a comment
sschuberth
left a comment
There was a problem hiding this comment.
I'm a bit unclear about the goal of this PR. Maybe we can discuss this in a developer meeting.
|
|
||
| companion object { | ||
| // Used by test utils. | ||
| } |
There was a problem hiding this comment.
Please, no such test-only changes in production code, esp. as it's just for small convenience functions.
There was a problem hiding this comment.
Can you please propose an alternative. It's not obvious to me at all why this is a problem.
There was a problem hiding this comment.
Can you please propose an alternative.
Why not create top-level "constants" / functions in reporter test fixtures?
It's not obvious to me at all why this is a problem.
IMO it's a code smell to have stuff that would not be needed if tests were written differently. I.e. if tests were changed, it's easy to forget about removing this again. Tests should be written against the API of production code, and not production code be altered for the sole purpose of testing convenience.
| implementation(projects.plugins.licenseFactProviders.scancodeLicenseFactProvider) | ||
| implementation(projects.plugins.licenseFactProviders.spdxLicenseFactProvider) |
There was a problem hiding this comment.
I believe we should avoid depending on plugin implementations in generic code as much as possible. It's already somewhat of a smell that we had api(projects.plugins.versionControlSystems.gitVersionControlSystem) before.
There was a problem hiding this comment.
Do you have a proposal where to put the helper functions?
There was a problem hiding this comment.
It's already somewhat of a smell that we had
api(projects.plugins.versionControlSystems.gitVersionControlSystem)before.
See #11976 in that regard.
There was a problem hiding this comment.
Do you have a proposal where to put the helper functions?
Reporter test fixtures seem to be a better fit.
It's been discussed in core dev slack. Maybe this clarifies it. |
fviernau
force-pushed
the
license-fact-provider-composite
branch
4 times, most recently
from
d7cec5f to
b8a1a3b
Compare
fviernau
force-pushed
the
license-fact-provider-composite
branch
from
b8a1a3b to
63b4de9
Compare
fviernau
changed the title
@sschuberth the idea for separation of APIs was described in second commit's message. However, to make things more clear, I've added another commit on top which already enhances the API as needed in scope of #11668. This should explain it all. |
Simplify the diff of an upcoming change. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
The `interface LicenseFactProvider` is implemented by plugins and also
used internally from various reporters. So, it should be suitable nicely
for both, the plugins and the calling code of the interface.
In an upcoming enhancement, it cannot work nicely for both anymore.
Prepare for that by no more implementing `LicenseFactProvider` with
`CompositeLicenseFactProvider`, to have clean separation of the APIs of
the plugins and of their internal use.
Note: `getNonBlankLicenseText()` has to be moved to
`CompositeLicenseFactProvider`, because that class is now the
internal API for code using the providers. That function is used
by the Freemarker template reporter scripts. As there is no
function implementation left in `LicenseFactProvider`, it is
turned back into an interface again.
Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
Implement the following heuristic for obtaining a license text for a given `licenseId` and optinal `id` in the reporter facing API, namely in `CompositeLicenseFactProvider`: 1. Find the first configured provider which has an id-specific license, and if found return the corresponding text. 2. Otherwise, find the first configured provider which has a non-id-specic license text, and if found return the corresponding text, or `null` otherwise. This is why the `LicenseFactProvider` is enhanced with functions, which allow to distinguish whether the obtained license text is id-specific. Do so by adding separate `interface` functions, for clarity and for easier implementation. Furthermore, provide default implementation, so that providers which do not support id-specific license texts do not need to care at all. This prepares for enhancing the provider used for ORT's "custom license texts dir" to support id-specific license text overrides. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
fviernau
force-pushed
the
license-fact-provider-composite
branch
from
45f92a4 to
587a321
Compare
| /** Return a non-blank license text for the given [licenseId] and [id], or `null` if no valid text is available. */ | ||
| @Deprecated("Java-only API", level = DeprecationLevel.HIDDEN) | ||
| @JvmName("getLicenseText") | ||
| fun getNonBlankLicenseText(licenseId: String, id: Identifier? = null): String? = getLicenseText(licenseId, id)?.text |
| fun getNonBlankLicenseText(licenseId: String, id: Identifier? = null): String? = getLicenseText(licenseId, id)?.text | ||
|
|
||
| /** Return `true´ if this provider has a license text for the given [licenseId] and [id]. */ | ||
| fun hasLicenseText(licenseId: String, id: Identifier? = null): Boolean { |
3 participants
At first, separate the plugin interface and the interface used by the reporters, to be able to dedicate the design to
each of the use cases separately. And then, enhance the API, so that id-specific license texts can be provided (optionally) and consumed (optionally).
Part of #11668.